Skip to content

Add integration tests for site csp report #1117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 9, 2019
Merged

Add integration tests for site csp report #1117

merged 1 commit into from
Sep 9, 2019

Conversation

mukeshk
Copy link
Contributor

@mukeshk mukeshk commented Sep 7, 2019 •
edited by php-coder
Loading

Fix #1094

@mukeshk mukeshk requested a review from php-coder as a code owner September 7, 2019 17:44
@mystamps-bot
Copy link

mystamps-bot commented Sep 7, 2019 •
edited
Loading

1 Error
🚫 robotframework-maven-plugin error in src/test/robotframework/account/registration/logic.robot#L11-L23:
Test case After account creation an e-mail with activation link should be send fails with message:
Length of ‘[]’ should be 1 but is 0
2 Warnings
⚠️ danger check: pull request description doesn’t contain a link to original issue.
Consider adding a comment in the following format: Addressed to #XXX where XXX is an issue number
⚠️ danger check: branch gh_1094_site_csp_report does not comply with our best practices. Branch name should use the following scheme: ghXXX_meaningful-name where XXX is an issue number. Next time, please, use this scheme :)
1 Message
📖 robotframework-maven-plugin reported about 1 error. Please, fix it. See also: https://github.com/php-coder/mystamps/wiki/integration-tests

Generated by 🚫 Danger

@mukeshk
Copy link
Contributor Author

mukeshk commented Sep 7, 2019

I am getting 400 on local machine, here it's reporting 204. I will check again

@mukeshk
Copy link
Contributor Author

mukeshk commented Sep 8, 2019

In the controller, the method is annotated with
@PostMapping(SiteUrl.CSP_REPORTS_HANDLER) @ResponseStatus(HttpStatus.NO_CONTENT) public void handleReport(

@mukeshk
Copy link
Contributor Author

mukeshk commented Sep 8, 2019

Do we need to change the @ResponseStatus(HttpStatus.NO_CONTENT)?

@php-coder
Copy link
Owner

Do we need to change the @ResponseStatus(HttpStatus.NO_CONTENT)?

No, as this is an expected behaviour -- the controller should report about success but 200 requires a body and 204 is a response that doesn't require a body. I've also updated an original issue where I mistakenly wrote 201 instead of 204.

@mukeshk
Copy link
Contributor Author

mukeshk commented Sep 8, 2019 via email

php-coder
php-coder requested changes Sep 8, 2019
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 👍
See my comments.

@mukeshk mukeshk force-pushed the gh_1094_site_csp_report branch from 05c7c9d to 99e8d7d Compare September 8, 2019 13:14
@mukeshk mukeshk changed the title [WIP]test(site/csp/report): verify csp request is accepted. test(site/csp/report): verify csp request is accepted. Sep 8, 2019
@mukeshk mukeshk changed the title test(site/csp/report): verify csp request is accepted. Add integration tests for site csp report Sep 8, 2019
php-coder
php-coder requested changes Sep 8, 2019
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments and also could you, please, modify the first line of commit message to be:

test(/site/csp/reports): add integration test.

@php-coder
Copy link
Owner

P.S. One more thing -- remove original TODO comment.

@mukeshk mukeshk force-pushed the gh_1094_site_csp_report branch from 99e8d7d to f0e5a9c Compare September 9, 2019 04:19
php-coder
php-coder reviewed Sep 9, 2019
View reviewed changes
php-coder
php-coder requested changes Sep 9, 2019
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!
Only one tiny bit is missing.

@mukeshk mukeshk force-pushed the gh_1094_site_csp_report branch from f0e5a9c to fb2cbbd Compare September 9, 2019 10:44
@php-coder php-coder merged commit 6013f08 into php-coder:master Sep 9, 2019
@mukeshk mukeshk deleted the gh_1094_site_csp_report branch September 9, 2019 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@php-coder php-coder php-coder approved these changes

Assignees
No one assigned
Labels
resolution/merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

/site/csp/reports: add integration tests
3 participants
@mukeshk @mystamps-bot @php-coder